-
Notifications
You must be signed in to change notification settings - Fork 27
Conversation
@@ -0,0 +1,156 @@ | |||
//! Escape strings for use in ostree refs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, perhaps this should go into https://github.com/ostreedev/ostree-rs - but that takes us down the new road of adding Rust-native API. Which...well, we'll visit ostreedev/ostree#2427 at some point.
//! | ||
//! This escaping scheme uses `_` in a similar way as a `\` character is | ||
//! used in Rust unicode escaped values. For example, `:` is `_3A_` (hexadecimal). | ||
//! Because the empty path is not valid, `//` is escaped as `/_2F_` (i.e. the second `/` is escaped). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fear that the rules for handling /
are too weird and will lead to future footguns, as you can see trying to escape a//
vs a/b/
vs a/b/c
(and similar).
I think the problem is that this logic is conflating an input character /
with the ostree fragments separator /
. Per ostree regexp, those two are not the same thing. We do introduce ambiguity here.
My suggestions here would be:
- always escape all
/
from the input string - clarify that the input stream is a single fragment, not a full ref (i.e. one can't do
docker_name/my_ostree_branch
) - if there is a need to encode a ref with multiple fragments, take them as already split inputs through a string slice (i.e.
["docker_name", "my_ostree_branch"]
.
Same for the unescape counterpart. With the current logic, once you get back an unescaped a/b/c
it's ambiguous whether the initial input was a single docker name or three ostree fragments or something inbetween.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way the container code uses this, it is unambiguous - see this https://github.com/ostreedev/ostree-rs-ext/pull/99/files#diff-d618b05deec14d83f95c25b128171241e11614efec182d232e6ae46a8f4c09feR21 - basically because we have a static string as prefix, and the rest is assumed defined/required to be escaped.
I think it's at least somewhat nicer to see /
literally, you just learn to read _3A_
as :
. Here's a random paste from my test VM:
[root@cosa-devsh tmp]# ostree refs
ostree/container/blob/sha256_3A_0f79a34559f64a74b08fd5bbad34072f8c1a7784f33bd750d878c7f387f1b5a1
ostree/container/blob/sha256_3A32d62d73008776158de3532b560bb4d1c087f9600eecc732594a5a31cdccaf5b
ostree/container/blob/sha256_3A_32d62d73008776158de3532b560bb4d1c087f9600eecc732594a5a31cdccaf5b
ostree/container/blob/sha256_3A0f79a34559f64a74b08fd5bbad34072f8c1a7784f33bd750d878c7f387f1b5a1
ostree/container/blob/sha256_3A655734cf6267d3ba1506d4dac15329679216c9169093a5a99b60b7484e4320d4
ostree/1/1/2
ostree/1/1/1
ostree/container/image/docker_3A_/_2F_quay_2E_io/cgwalters/fcos-derivation-example_3A_latest
ostree/1/1/0
ostree/container/blob/sha256_3A_809c66ca43fa2ac72c15a83adb27b1e1794a7f5ae8463b3a8c559164a9201d8b
ostree/container/blob/sha256_3A_655734cf6267d3ba1506d4dac15329679216c9169093a5a99b60b7484e4320d4
ostree/container/blob/sha256_3A809c66ca43fa2ac72c15a83adb27b1e1794a7f5ae8463b3a8c559164a9201d8b
ostree/container/blob/sha256_3A5707006e0d4d228ea1011b85c431727af86bf46309615b04555e95fb3849dab7
test
[root@cosa-devsh tmp]#
With your proposal of escaping individual fragments...the calling app would need to have rules/schema for which portions of a ref are escaped. I guess I could imagine doing that, but I am having trouble thinking of a strong use case for that versus the "prefixing" approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually we should canonicalize docker://
to registry:
I think instead of the other way around. Then it'd be:
ostree/container/image/registry_3A_quay_2E_io/cgwalters/fcos-derivation-example_3A_latest
which I think is nicer than reading
ostree/container/image/registry_3A_quay_2E_io_2F_cgwalters_2F_fcos-derivation-example_3A_latest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the additional context! It looks the API you are aiming for is perhaps something like $input
-> prefix/$escaped-input
and back? But then is there anything preventing people to do prefix/$escaped-input/arbitrarystuff
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I reworked this to require a prefix to better demonstrate best practice.
Perhaps it'd be better to fully enshrine this as an APIs which accept a prefix, and escape/unescape the rest. The |
c18bf9b
to
d72c474
Compare
Nevermind still more to do |
d72c474
to
048f03e
Compare
OK, this should be better now. |
c291a76
to
452215c
Compare
Prep for work on the new container module, where we want to store container image references (e.g. `docker://quay.io/coreos/fedora`) as ostree refs. Several bits of that are not valid in ostree refs, such as the `:` or the double `//` (which would be an empty filesystem path). This escaping scheme uses `_` in a similar way as a `\` character is used in other syntax. For example, `:` is `_3A_` (hexadecimal). `//` is escaped as `/_2F_` (i.e. the second `/` is escaped).
452215c
to
3a19057
Compare
OK, assuming comments here were addressed, if anyone has anything further we can do post-merge fixes! |
Prep for work on the new container module, where we want to store
container image references (e.g.
docker://quay.io/coreos/fedora
)as ostree refs. Several bits of that are not valid in ostree refs,
such as the
:
or the double//
(which would be an empty filesystem path).This escaping scheme uses
_
in a similar way as a\
character isused in other syntax. For example,
:
is_3A_
(hexadecimal).//
is escaped as/_2F_
(i.e. the second/
is escaped).